Skip to content

chore: fix accumulation logic for PrimeField vectors#416

Open
mdqst wants to merge 3 commits into
microsoft:mainfrom
mdqst:patch-1
Open

chore: fix accumulation logic for PrimeField vectors#416
mdqst wants to merge 3 commits into
microsoft:mainfrom
mdqst:patch-1

Conversation

@mdqst

@mdqst mdqst commented Oct 13, 2025

Copy link
Copy Markdown

updated the accumulation from using .sum() to an explicit .fold(F::zero(), |acc, x| acc + x).
this ensures correct behavior for all F: PrimeField types and resolves the issue with summing field elements.

@srinathsetty

Copy link
Copy Markdown
Collaborator

Can you say more on what the issue with using sum is?

@mdqst

mdqst commented Oct 15, 2025

Copy link
Copy Markdown
Author

srinathsetty, sum() can be a bit tricky with field types - it goes through the Sum trait, which doesn’t always behave as expected for PrimeField elements (especially with refs and generic bounds). Using an explicit fold(F::zero(), |acc, x| acc + x) makes the accumulation unambiguous and guarantees it stays within the field operations. just safer and clearer

@srinathsetty srinathsetty requested a review from Copilot October 16, 2025 13:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the accumulation logic for PrimeField vectors by replacing .sum() with an explicit .fold(F::zero(), |acc, x| acc + x) operation. This change ensures correct behavior when summing field elements in the sparse matrix-vector multiplication implementation.

Key Changes:

  • Updated the accumulation method in SparseMatrix::multiply to use explicit folding instead of .sum()

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mdqst

mdqst commented Oct 19, 2025

Copy link
Copy Markdown
Author

srinathsetty, lets merge it

@srinathsetty

Copy link
Copy Markdown
Collaborator

srinathsetty, lets merge it

Can we please fix CI failures?

@mdqst

mdqst commented Dec 5, 2025

Copy link
Copy Markdown
Author

srinathsetty done

@mdqst

mdqst commented Dec 25, 2025

Copy link
Copy Markdown
Author

srinathsetty lets merge it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants